feat: DPLPMTUD #1903
Merged
feat: DPLPMTUD #1903
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
larseggert
changed the title
As far as I can tell the trait function Does that make sense @larseggert? |
larseggert
commented
May 14, 2024
mxinden
reviewed
May 14, 2024
Benchmark resultsPerformance differences relative to 7d610ed. coalesce_acked_from_zero 1+1 entries: Change within noise threshold. time: [193.61 ns 194.06 ns 194.53 ns]
change: [+0.0719% +0.4048% +0.7829%] (p = 0.02 < 0.05)
coalesce_acked_from_zero 3+1 entries: No change in performance detected. time: [234.67 ns 235.73 ns 237.08 ns]
change: [+0.6006% +2.3382% +6.2565%] (p = 0.06 > 0.05)
coalesce_acked_from_zero 10+1 entries: No change in performance detected. time: [233.27 ns 234.03 ns 234.96 ns]
change: [-0.1847% +0.3385% +0.8310%] (p = 0.21 > 0.05)
coalesce_acked_from_zero 1000+1 entries: No change in performance detected. time: [215.28 ns 217.58 ns 222.98 ns]
change: [-0.5398% +6.4762% +20.010%] (p = 0.44 > 0.05)
RxStreamOrderer::inbound_frame(): 💔 Performance has regressed. time: [120.23 ms 120.29 ms 120.37 ms]
change: [+1.1136% +1.1966% +1.2758%] (p = 0.00 < 0.05)
transfer/Run multiple transfers with varying seeds: 💚 Performance has improved. time: [55.737 ms 59.125 ms 62.506 ms]
thrpt: [63.994 MiB/s 67.653 MiB/s 71.766 MiB/s]
change:
time: [-53.858% -51.136% -48.358%] (p = 0.00 < 0.05)
thrpt: [+93.642% +104.65% +116.72%]
transfer/Run multiple transfers with the same seed: 💚 Performance has improved. time: [65.695 ms 70.817 ms 75.893 ms]
thrpt: [52.706 MiB/s 56.484 MiB/s 60.887 MiB/s]
change:
time: [-45.571% -41.490% -37.459%] (p = 0.00 < 0.05)
thrpt: [+59.896% +70.912% +83.726%]
1-conn/1-100mb-resp (aka. Download)/client: 💚 Performance has improved. time: [154.81 ms 160.68 ms 166.89 ms]
thrpt: [599.19 MiB/s 622.35 MiB/s 645.97 MiB/s]
change:
time: [-86.471% -85.914% -85.283%] (p = 0.00 < 0.05)
thrpt: [+579.47% +609.92% +639.15%]
1-conn/10_000-parallel-1b-resp (aka. RPS)/client: Change within noise threshold. time: [430.16 ms 433.60 ms 437.05 ms]
thrpt: [22.880 Kelem/s 23.063 Kelem/s 23.247 Kelem/s]
change:
time: [-2.5302% -1.4809% -0.4847%] (p = 0.00 < 0.05)
thrpt: [+0.4870% +1.5031% +2.5959%]
1-conn/1-1b-resp (aka. HPS)/client: 💚 Performance has improved. time: [43.569 ms 44.108 ms 44.652 ms]
thrpt: [22.395 elem/s 22.671 elem/s 22.952 elem/s]
change:
time: [-3.7644% -2.5274% -1.2515%] (p = 0.00 < 0.05)
thrpt: [+1.2674% +2.5930% +3.9116%]
Client/server transfer resultsTransfer of 33554432 bytes over loopback.
|
larseggert
commented
May 21, 2024
There was a problem hiding this comment.
I'm not seeing PMTUD tests, which would be necessary for this.
The big question I have is the one that Christian makes about PTMUD generally: how do you know that the bytes you use on PMTUD pay you back?
There is probably a case for sending probes when you have spare sending capacity and nothing better to send. Indeed, successfully probing will let us push congestion windows up more and could even improve performance.
What I'm seeing here displaces other data. I'd like to see something that doesn't do that. There's a fundamental problem that needs analysis though. You can't predict that a connection will be used for uploads, so you don't know when probes will really help. I see a few cases:
- The connection is short-lived or low volume. Probes are strictly wasteful.
- The connection is long-lived and high volume, with ample idle time for probing. Probes can use gaps. This might be a video stream, where probing can fit into a warmup period. Probes are therefore easy and super-helpful.
- The connection exists only to support a smaller upload. The upload is small enough that probes are wasteful.
- The connection exists only to support a larger upload. The upload is large enough that spending bytes on probing early on is a good investment.
Case 1 and 2 are easy to deal with. We could probe on an idle connection and tolerate a small amount of waste for case 1 if it makes case 2 appreciably better.
The split between 3 and 4 is rough. There is an uncertain zone between the two as well where some probing is justified, but successive rounds of probing might be wasteful as the throughput gain over the remaining time diminishes relative to the wasted effort of extra probes.
Right now, you don't send real data in probes. You are effectively betting on the probes being lost. But you could send data, which would reduce the harm in case 3. It might even make the code slightly simpler.
Co-authored-by: Martin Thomson <mt@lowentropy.net> Signed-off-by: Lars Eggert <lars@eggert.org>
Co-authored-by: Max Inden <mail@max-inden.de> Signed-off-by: Lars Eggert <lars@eggert.org>
Co-authored-by: Martin Thomson <mt@lowentropy.net> Signed-off-by: Lars Eggert <lars@eggert.org>
mxinden
added a commit
to mxinden/neqo
that referenced
this pull request
Jul 10, 2024
Follow-up on mozilla#1903 (comment)
mxinden
added a commit
to mxinden/neqo
that referenced
this pull request
Jul 10, 2024
Follow-up on mozilla#1903 (comment)
mxinden
added a commit
to mxinden/neqo
that referenced
this pull request
Jul 10, 2024
Follow-up on mozilla#1903 (comment)
Follow-up on mozilla#1903 (comment)
Closed
`Probe` is a small simple enum on the stack, thus convention is to implement `Copy` instead of only `Clone` with a call to `clone()`. The following helped me in the past: > When should my type be Copy? > > Generally speaking, if your type can implement Copy, it should. Keep in mind, > though, that implementing Copy is part of the public API of your type. If the > type might become non-Copy in the future, it could be prudent to omit the Copy > implementation now, to avoid a breaking API change. https://doc.rust-lang.org/std/marker/trait.Copy.html#when-should-my-type-be-copy
moz-v2v-gh
pushed a commit
to mozilla/gecko-dev
that referenced
this pull request
Sep 16, 2024
…iewers,kershaw,janerik This commit adds four Glean probes: - http3_udp_datagram_segment_size_sent - http3_udp_datagram_segment_size_received - http3_udp_datagram_size_received - http3_udp_datagram_num_segments_received Given the performance impact tracking Glean metrics in the UDP hot path, see https://phabricator.services.mozilla.com/D216034#7453056, this commit introduces a sample buffer per metric. This will enable us to measure the impact of: - Implementation of Packetization Layer Path MTU Discovery for Datagram Transports (RFC 8899) [in Neqo](mozilla/neqo#1903) - [Fast UDP for Firefox](https://bugzilla.mozilla.org/show_bug.cgi?id=1901292) Differential Revision: https://phabricator.services.mozilla.com/D216034
i3roly
pushed a commit
to i3roly/firefox-dynasty
that referenced
this pull request
Sep 17, 2024
…iewers,kershaw,janerik This commit adds four Glean probes: - http3_udp_datagram_segment_size_sent - http3_udp_datagram_segment_size_received - http3_udp_datagram_size_received - http3_udp_datagram_num_segments_received Given the performance impact tracking Glean metrics in the UDP hot path, see https://phabricator.services.mozilla.com/D216034#7453056, this commit introduces a sample buffer per metric. This will enable us to measure the impact of: - Implementation of Packetization Layer Path MTU Discovery for Datagram Transports (RFC 8899) [in Neqo](mozilla/neqo#1903) - [Fast UDP for Firefox](https://bugzilla.mozilla.org/show_bug.cgi?id=1901292) Differential Revision: https://phabricator.services.mozilla.com/D216034
jamienicol
pushed a commit
to jamienicol/gecko
that referenced
this pull request
Sep 17, 2024
…iewers,kershaw,janerik This commit adds four Glean probes: - http3_udp_datagram_segment_size_sent - http3_udp_datagram_segment_size_received - http3_udp_datagram_size_received - http3_udp_datagram_num_segments_received Given the performance impact tracking Glean metrics in the UDP hot path, see https://phabricator.services.mozilla.com/D216034#7453056, this commit introduces a sample buffer per metric. This will enable us to measure the impact of: - Implementation of Packetization Layer Path MTU Discovery for Datagram Transports (RFC 8899) [in Neqo](mozilla/neqo#1903) - [Fast UDP for Firefox](https://bugzilla.mozilla.org/show_bug.cgi?id=1901292) Differential Revision: https://phabricator.services.mozilla.com/D216034
gecko-dev-updater
pushed a commit
to marco-c/gecko-dev-wordified-and-comments-removed
that referenced
this pull request
Sep 20, 2024
…iewers,kershaw,janerik This commit adds four Glean probes: - http3_udp_datagram_segment_size_sent - http3_udp_datagram_segment_size_received - http3_udp_datagram_size_received - http3_udp_datagram_num_segments_received Given the performance impact tracking Glean metrics in the UDP hot path, see https://phabricator.services.mozilla.com/D216034#7453056, this commit introduces a sample buffer per metric. This will enable us to measure the impact of: - Implementation of Packetization Layer Path MTU Discovery for Datagram Transports (RFC 8899) [in Neqo](mozilla/neqo#1903) - [Fast UDP for Firefox](https://bugzilla.mozilla.org/show_bug.cgi?id=1901292) Differential Revision: https://phabricator.services.mozilla.com/D216034 UltraBlame original commit: 16d3f312970444dd8bc8af65629cbef7d7dbcd62
gecko-dev-updater
pushed a commit
to marco-c/gecko-dev-wordified
that referenced
this pull request
Sep 20, 2024
…iewers,kershaw,janerik This commit adds four Glean probes: - http3_udp_datagram_segment_size_sent - http3_udp_datagram_segment_size_received - http3_udp_datagram_size_received - http3_udp_datagram_num_segments_received Given the performance impact tracking Glean metrics in the UDP hot path, see https://phabricator.services.mozilla.com/D216034#7453056, this commit introduces a sample buffer per metric. This will enable us to measure the impact of: - Implementation of Packetization Layer Path MTU Discovery for Datagram Transports (RFC 8899) [in Neqo](mozilla/neqo#1903) - [Fast UDP for Firefox](https://bugzilla.mozilla.org/show_bug.cgi?id=1901292) Differential Revision: https://phabricator.services.mozilla.com/D216034 UltraBlame original commit: 16d3f312970444dd8bc8af65629cbef7d7dbcd62
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This implements a simplified variant of PLDPMTUD (aka RFC8899), which by default probes for increasingly larger PMTUs using an MTU table.
There is currently no attempt to repeat the PMTUD at intervals.There is also no attempt to detect PMTUs that are in between values in the table.There is no attempt to handle the case where the PMTU shrinks.A lot of the existing tests (~50%) break when PMTUD is enabled, so this PR disables it by default. New tests that cover PMTUD were added to this PR.
Fixes #243